Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: load config defaults if not specified in existing config #1544

Merged
merged 10 commits into from
Jan 23, 2024

Conversation

zivkovicmilos
Copy link
Member

@zivkovicmilos zivkovicmilos commented Jan 17, 2024

Description

This PR fixes the flow of loading Gno configuration files.
Previously, configuration files were loaded without applying default values, causing to funky behavior.

Additionally, this PR drops the config TOML template, instead relying on the TOML package itself to marshal.

Issue spotted in #1238

Note:
I've tried to keep the original functionality of LoadOrMakeConfigWithOptions, because if this is changed or broken up in its current state, things start breaking across dimensions.

Funky behavior that's fixed:
The EventStore fields were not being saved in the original config (since we utilized a template for it, and this was not present in the template). Now, config files should properly save on disk the event store configuration.

Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests
  • Added new benchmarks to generated graphs, if any. More info here.

@zivkovicmilos zivkovicmilos added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label Jan 17, 2024
@zivkovicmilos zivkovicmilos self-assigned this Jan 17, 2024
@github-actions github-actions bot added 📦 🌐 tendermint v2 Issues or PRs tm2 related and removed 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Jan 17, 2024
@github-actions github-actions bot added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label Jan 17, 2024
Copy link

codecov bot commented Jan 17, 2024

Codecov Report

Attention: 52 lines in your changes are missing coverage. Please review.

Comparison is base (6112ffd) 55.95% compared to head (33a1a2f) 55.92%.
Report is 3 commits behind head on master.

Files Patch % Lines
tm2/pkg/bft/config/config.go 0.00% 42 Missing ⚠️
tm2/pkg/bft/config/toml.go 0.00% 9 Missing ⚠️
tm2/pkg/bft/state/eventstore/types/config.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1544      +/-   ##
==========================================
- Coverage   55.95%   55.92%   -0.04%     
==========================================
  Files         435      435              
  Lines       66306    66612     +306     
==========================================
+ Hits        37101    37252     +151     
- Misses      26303    26460     +157     
+ Partials     2902     2900       -2     
Flag Coverage Δ
go-1.21.x ∅ <ø> (∅)
misc ∅ <ø> (∅)
misc-_test.genstd ∅ <ø> (∅)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zivkovicmilos zivkovicmilos marked this pull request as ready for review January 17, 2024 13:31
@zivkovicmilos zivkovicmilos requested review from jaekwon, moul and a team as code owners January 17, 2024 13:31
Copy link
Contributor

@deelawn deelawn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done 👍

tm2/pkg/bft/config/config_test.go Outdated Show resolved Hide resolved
tm2/pkg/bft/config/config_test.go Outdated Show resolved Hide resolved
tm2/pkg/bft/config/config.go Outdated Show resolved Hide resolved
tm2/pkg/bft/config/config.go Outdated Show resolved Hide resolved
tm2/pkg/bft/config/config.go Show resolved Hide resolved
tm2/pkg/bft/config/toml.go Show resolved Hide resolved
@zivkovicmilos zivkovicmilos requested review from a team and piux2 as code owners January 22, 2024 10:55
Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please go ahead and resolve&merge after addressing 👍

tm2/pkg/bft/state/eventstore/types/config.go Show resolved Hide resolved
tm2/pkg/bft/config/toml_test.go Outdated Show resolved Hide resolved
@zivkovicmilos zivkovicmilos added breaking change Functionality that contains breaking changes and removed breaking change Functionality that contains breaking changes labels Jan 23, 2024
@thehowl thehowl merged commit d1ead00 into gnolang:master Jan 23, 2024
198 of 199 checks passed
leohhhn pushed a commit to leohhhn/gno that referenced this pull request Jan 31, 2024
…nolang#1544)

## Description

This PR fixes the flow of loading Gno configuration files. 
Previously, configuration files were loaded without applying default
values, causing to funky behavior.

Additionally, this PR drops the config TOML template, instead relying on
the TOML package itself to marshal.

Issue spotted in gnolang#1238 

Note:
I've tried to keep the original functionality of
`LoadOrMakeConfigWithOptions`, because if this is changed or broken up
in its current state, things start breaking across dimensions.

Funky behavior that's fixed:
The `EventStore` fields were not being saved in the original config
(since we utilized a template for it, and this was not present in the
template). Now, config files should properly save on disk the event
store configuration.

<details><summary>Contributors' checklist...</summary>

- [x] Added new tests, or not needed, or not feasible
- [x] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [x] Updated the official documentation or not needed
- [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [x] Added references to related issues and PRs
- [ ] Provided any useful hints for running manual tests
- [ ] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related
Projects
Status: Done
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants